RFC about certificate handling #24
Conversation
Adding RFC that describes how certificate handling should be done inside of the KW project. Signed-off-by: Flavio Castelli <fcastelli@suse.com>
flavio
force-pushed
the
certificate-handling
branch
from
8fe24d0
to
8c9dd94
Compare
flavio
changed the title
Co-authored-by: Víctor Cuadrado Juan <2196685+viccuad@users.noreply.github.com> Signed-off-by: Flavio Castelli <flavio@castelli.me>
| ## Proposed Design | ||
|
|
||
| We would like to get rid of CertManager. To do that we need to change how certificates are | ||
| managed for the `kubewarden-controller`. | ||
| Going forward, the controller will also take care of generating the certificate used by the kubewarden-controller. | ||
| This certificate is going to be signed by the CA which is already created by the controller. |
There was a problem hiding this comment.
Will we remove it completely or make it optional? I would like to double check that because the original issue around this topic says to turn it optional
There was a problem hiding this comment.
I agree on having it optional, and disabled by default.
There was a problem hiding this comment.
What would be the advantage of having this certificate generated by CertManager? I mean, we're still generating all the internal ones by ourselves.
I would prefer to drop the CertManager dependency entirely, just to reduce the support matrix and keep the code as simple as possible
There was a problem hiding this comment.
@flavio does this comment still stand? I thought we wanted to still support cert-manager, but not as default.
There was a problem hiding this comment.
Last week @jvanz, @fabriziosestito and agreed to focus on removing the cert-manager dependency. We can make it optional in the future if there's some request coming from the community.
| The reconciliation loop is also triggered every X seconds as a way to cope with possible glitches | ||
| with the event notification system. |
There was a problem hiding this comment.
Where is this time based trigger setup? Is this done by kube-builder? I cannot see this in the code. =(
There was a problem hiding this comment.
Good question, we're currently using controller-runtime version 0.14.6.
When creating the Manager we give to it some configuration Options, see here
One of the values of the Manager Options is called SyncPeriod. Since we don't specify anything, we will have an automatic sync interval of 10 hours. Meaning, even if there's no event affecting one of our watched resources, we will still have a sync every 10 hours.
This configuration parameter has been moved to another location inside of the latest release of controller-runtime. Now this is one of the configuration values of the cache. The behaviour stays the same: every 10 hours you get a full sync.
We can rely on this 10 hours sync or we could be more explicit inside of our reconciliation loops by returning a reconcile.Result{RequeueAfter: t} instead of a reconcile.Result{}.
What do you think?
There was a problem hiding this comment.
I'd probably go with the latter. As the docstring states, we shouldn't change the SyncPeriod as it forces every object in the cache to be reconciled, and we might need a shorter window than 10 hours. Not a strong opinion, though.
There was a problem hiding this comment.
I would either leave things untouched, hence rely on the default sync value, or provide an explicit sync timer via the Result response
There was a problem hiding this comment.
I'd probably go with the latter.
Sorry I did not explain myself well, I meant that I have a small preference for the Result solution.
There was a problem hiding this comment.
No worries, that was clear to me. I also share the same feeling about how to move forward.
This change (being explicit about the sync time) should be tracked with a dedicated issue, which would be unrelated to this RFC
Now we know how to handle the renewal of the internal root CA. Signed-off-by: Flavio Castelli <fcastelli@suse.com>
This RFC describes the current state of certificate handling inside of Kubewarden and how this should be extended.